-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add option to clear etherscan cache #1807
Conversation
…lock data and etherscan data, respectively. before: ~/.foundry/cache/<chain>/block, ~/.foundry/cache/<chain>/etherscan after: ~/.foundry/cache/rpc/<chain>, ~/.foundry/cache/etherscan/<chain>
Hi folks! I'm a rust noob and still learning idiomatic rust so please feel free to nitpick to your heart's content. Also, I wanted to add some unit tests but wasn't sure exactly where to put them and couldn't find pre-existing tests for |
config/src/lib.rs
Outdated
@@ -1049,9 +1049,13 @@ impl Config { | |||
} | |||
if let Ok(entries) = cache_dir.as_path().read_dir() { | |||
for entry in entries.flatten().filter(|x| x.path().is_dir()) { | |||
let chain = Chain::from_str(&entry.file_name().to_string_lossy()).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of now, every entry inside the cache/rpc
directory is a valid chain_id. Maybe this changes in the future...
Is it a bad idea to use unwrap()
here instead of handling the exceptional case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this shouldn't happen but just to be safe we could filter here by Some and continue the loop otherwise?
config/src/lib.rs
Outdated
cache.chains.push(ChainCache { | ||
name: entry.file_name().to_string_lossy().into_owned(), | ||
blocks: Self::get_cached_blocks(&entry.path())?, | ||
block_explorer: Self::get_cached_block_explorer_data( | ||
&Config::foundry_etherscan_chain_cache_dir(chain).unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to above ... this will panic if $HOME
doesn't exist. It's a rare scenario, but my assumption is that unwrap()
is not ok here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should available for most OS but I agree, let's return an error in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added explicit handling with clear error message if this directory doesn't exist
for block in chain_path | ||
.read_dir()? | ||
.flatten() | ||
.filter(|x| x.file_type().unwrap().is_dir() && !x.file_name().eq("etherscan")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the "etherscan" filter as I changed the cache directory structure to hoist the etherscan cache outside of this directory.
NumberPrefix::Standalone(size) => { | ||
writeln!(f, "-️ {} ({:.1} B)", chain.name, size)?; | ||
} | ||
NumberPrefix::Prefixed(prefix, size) => { | ||
writeln!(f, "-️ {} ({:.1} {}B)", chain.name, size, prefix)?; | ||
} | ||
} | ||
match NumberPrefix::decimal(chain.block_explorer as f32) { | ||
NumberPrefix::Standalone(size) => { | ||
writeln!(f, "\t-️ Block Explorer ({:.1} B)\n", size)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the additional newline - it was cleaner to my eye to add an additional line to separate the etherscan data and the block data
#[derive(Debug)] | ||
pub struct ChainCache { | ||
/// The name of the chain | ||
pub name: String, | ||
|
||
/// A tuple containing block number and the block directory size in bytes | ||
pub blocks: Vec<(String, u64)>, | ||
|
||
/// The size of the block explorer directory in bytes | ||
pub block_explorer: u64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this could be block_explorer_size
. Could also be named etherscan_size
to keep it consistent with the flag --etherscan
. It's always difficult to determine whether to keep names specific to ethereum or generalize for all EVM chains.
Found some integration tests here so will add a few of my own! https://github.com/foundry-rs/foundry/blob/master/cli/tests/it/cmd.rs#L41 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks so much,
looking good already, just some smol nits.
re integration tests, they're useful for debugging but we should then ignore them otherwise we'll always clean our local cache if we run tests
config/src/lib.rs
Outdated
@@ -1049,9 +1049,13 @@ impl Config { | |||
} | |||
if let Ok(entries) = cache_dir.as_path().read_dir() { | |||
for entry in entries.flatten().filter(|x| x.path().is_dir()) { | |||
let chain = Chain::from_str(&entry.file_name().to_string_lossy()).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this shouldn't happen but just to be safe we could filter here by Some and continue the loop otherwise?
config/src/lib.rs
Outdated
cache.chains.push(ChainCache { | ||
name: entry.file_name().to_string_lossy().into_owned(), | ||
blocks: Self::get_cached_blocks(&entry.path())?, | ||
block_explorer: Self::get_cached_block_explorer_data( | ||
&Config::foundry_etherscan_chain_cache_dir(chain).unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should available for most OS but I agree, let's return an error in that case?
config/src/lib.rs
Outdated
let block_explorer_data_size = match Config::foundry_etherscan_chain_cache_dir(chain) { | ||
Some(cache_dir) => Self::get_cached_block_explorer_data(&cache_dir).unwrap(), | ||
None => 0, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make it consistently no unwrap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed all unwraps in favor of skipping or explicit errors. Also simplified the code by reusing the method to list a single cache entry when listing all cache entries.
f89fb53
to
89faba0
Compare
…eleting the etherscan cache data. e.g. forge cache clean --etherscan # deletes ~/.foundry/cache/etherscan forge cache clean mainnet --etherscan # deletes ~/.foundry/cache/etherscan/mainnet * test: Add tests for cache clean --etherscan
Oh and last thing, if you specify the On this note ^, I could use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks so much,
TIL ArgGroup
, let's add that for incompatible args @diligentcodoor
Is this working properly?
Doesn't seem to detect any chain now. Should we remove the |
A few more updates: @gakonst I believe it is working correctly. If it isn't, you should be getting an error message, rather than a silent non-print. What does your cache layout look like? The tricky thing about the cache integration tests is they share the filesystem state. So it's inconvenient for devs as you have to blow up your own cache and it's inconvenient for CI as you have to run them single-threaded. At the moment, they're really only useful to automate manual testing rather than ensuring correctness. On second thought, I can convert my integration test to a few unit tests for Lastly, I caught a bug calculating the cache size. The size was a shallow calculation rather than summing the files within subdirectories. Added a test for this as well. |
test: add integration test for 'forge cache ls' fix: calculate etherscan cache size based on all files in each subdirectory test: Unit test cache.to_string() used in 'forge cache ls'
…'forge cache clean'
I added a unit test to cover the discrepancy in cache output that you experienced @gakonst For future reference, is it better to wait until the PR is in a final state to squash commits? I know I keep making fixes and then overwriting previous commits and that may be difficult to see what has changed since a prior review. |
No need to squash before merging, we always do squash merge via Github so it's not required for you to do it! |
This is how my cache looks like:
|
I think you may not have run the code from this branch. The reason I think so is because the layout of the cache has changed. The code now expects:
Without the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's see how this works - should we also be using this for e.g. constructor-args
on forge create etc.?
closes #1477 |
Do you mean adding an option like I can open a separate issue to track this if you think it is a worthwhile use case. |
Motivation
Currently forge cache clean only clears RPC cache, but it would be beneficial to also be able to clear the Etherscan verification status cache.
Solves #1477
Solution
This PR:
Adds the
--etherscan
flag toforge cache clean
Displays the etherscan cache storage with
forge cache ls
: